Add validation to only allow Cassandra major version 3 by wallrj · Pull Request #321 · jetstack/navigator

wallrj · 2018-04-10T10:29:16Z

Only allow versions >= 3.0.0 && < 4.0.0
Disallow version changes in API updates (pending WIP: cassandra upgrade #277)
Document the supported versions.
Remove home made Cassandra version parsing code and replace with Hashicorp go-version which parses the looser form of version used by Cassandra.
Add github.com/hashicorp/go-version as a dependency.

Fixes: #320
Release note:

NONE

wallrj · 2018-04-10T11:07:11Z

/retest

munnerz · 2018-04-11T14:10:24Z

pkg/apis/navigator/validation/cassandra.go

+		case versionSpecGe:
+			if v.LessThan(opVersion) {
+				return false
+			}


Can we invert the implementation here?

i.e. versionSpecGe should run v.Greater and v.Equal and return true

I deleted this code and instead use github.com/hashicorp/go-version constraints.

munnerz · 2018-04-11T14:10:52Z

pkg/apis/navigator/validation/cassandra.go

+			if opVersion.Equal(v) {
+				return false
+			}
+			if opVersion.LessThan(v) {


We switch to testing opVersion instead of v here - can we be consistent for readability?

I deleted this code and instead use github.com/hashicorp/go-version constraints.

munnerz · 2018-04-11T14:11:34Z

pkg/apis/navigator/validation/cassandra.go

+	for op, opVersion := range vs {
+		out = append(out, fmt.Sprintf("%s %s", op, opVersion))
+	}
+	return strings.Join(out, ", ")


Maps are not consistently ordered, so this version string will be unstable. Can we make it stable?

I deleted this code and instead use github.com/hashicorp/go-version constraints.

munnerz · 2018-04-11T14:12:17Z

pkg/apis/navigator/validation/cassandra.go

+	versionSpecLt: version.New("4.0.0"),
+}
+
+func (vs versionSpec) Check(v *version.Version) bool {


Maybe a quick unit test for this function?

Switched to github.com/hashicorp/go-version constraints which already has unit tests.

wallrj · 2018-04-25T15:12:35Z

Hey @munnerz.

Please take another look.

I replaced my home grown constraints checking code with github.com/hashicorp/go-version constraints.
Which seems to be more popular and better tested than the alternative https://github.com/Masterminds/semver
Also used Hashicorp version instead of coreos semver in the Cassandra Version type.
And added generated DeepCopy functions to that, as discussed.

I can follow this up with PRs to

use the same custom Version type for Elastic search.
have the controller perform an additional check for supported versions to handle the case where the API server may have been upgraded but the controller has not.

wallrj · 2018-04-26T07:49:27Z

/retest

munnerz · 2018-04-26T10:26:55Z

pkg/cassandra/version/version.go

+	if err != nil {
+		return err
+	}
+	v.versionString = s


Do we need to keep the versionString in memory? Surely semver.String will be more up to date?

We need it because we want to remember the format of the originally supplied version string.
So if the Cassandra node reports version 3.11 via nodetool and a JMX query, we want to report that exact version in the Pilot.Status.Cassandra.Version rather than the semver 3.11.0.

We also don't want a user to submit CassandraCluster.Spec.Version: 3.11 and then have that change to 3.11.0 when they do kubectl get CassandraCluster -o yaml.

However, when we infer the Docker image tag, we use the semver, because we don't want docker to simply pull the latest 3.11 image from Dockerhub.

munnerz · 2018-04-26T10:28:22Z

pkg/cassandra/version/version.go

-	"github.com/coreos/go-semver/semver"
+	semver "github.com/hashicorp/go-version"
 )



Can we move this file out of pkg/cassandra and into something like pkg/api/util? This is an API type, but it isn't by default part of an API group (hence it isn't in apis).

It'd be good to denote it as such in the file structure, to make it clear this isn't specific to just cassandra (and also denotes that this type may be exposed in an api)

munnerz · 2018-04-26T10:31:17Z

pkg/cassandra/version/version_test.go

+	t.Run(
+		"zero value",
+		func(t *testing.T) {
+			t.Log(version.Version{}.DeepCopy())


This doesn't test anything, or ever fail afaict

It tests that a Zero value can be DeepCopied.
It originally caused a panic before I added a check for nil pointer in the implementation.
I'll leave it for now.

munnerz · 2018-04-26T10:31:53Z

pkg/cassandra/version/version.go

+	if v.semver == nil {
+		return Version{}
+	}
+	return *New(v.String())


munnerz · 2018-04-26T10:33:29Z

Gopkg.toml

+
+[[constraint]]
+  branch = "master"
+  name = "github.com/hashicorp/go-version"


Don't think this needs to be added as a constraint given we are constraining it to master anyway. A simple dep ensure without adding this block should yield the same result, sans this change

wallrj · 2018-04-26T16:18:49Z

/retest

wallrj · 2018-04-26T16:19:28Z

The navigator-e2e-v1-9 E2E test failure was a flake, described here: jetstack/test-infra#180

munnerz · 2018-05-08T11:29:02Z

/lgtm
/approve

jetstack-bot · 2018-05-08T11:29:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [munnerz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jetstack-bot added release-note-none size/L labels Apr 10, 2018

jetstack-bot requested a review from munnerz April 10, 2018 10:29

munnerz reviewed Apr 11, 2018

View reviewed changes

wallrj added 6 commits April 25, 2018 10:45

Add github.com/hashicorp/go-version

304bb3c

Add API validation to only allow Cassandra v3

0418c2b

Add DeepCopy

2f7eb7e

Document the supported versions

4c365f9

Add API validation to prevent version changes

67450fe

dep ensure -no-vendor

f284870

wallrj force-pushed the 320-version-validation branch from 9af030e to f284870 Compare April 25, 2018 15:03

jetstack-bot added size/XXL and removed size/L labels Apr 25, 2018

Handle DeepCopy of zero value versions

b034203

munnerz reviewed Apr 26, 2018

View reviewed changes

wallrj added 2 commits April 26, 2018 16:15

Remove constraint

930bf6f

Make the cassandra version package an api sub-package

635c5fc

munnerz added the feature/upgrade label May 8, 2018

jetstack-bot assigned munnerz May 8, 2018

jetstack-bot added the lgtm label May 8, 2018

jetstack-bot added the approved label May 8, 2018

jetstack-bot merged commit 4eb5cb2 into jetstack:master May 8, 2018

Conversation

wallrj commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wallrj commented Apr 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wallrj commented Apr 25, 2018

Uh oh!

wallrj commented Apr 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wallrj commented Apr 26, 2018

Uh oh!

wallrj commented Apr 26, 2018

Uh oh!

munnerz commented May 8, 2018

Uh oh!

jetstack-bot commented May 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wallrj commented Apr 10, 2018 •

edited

Loading